Skip to content

feat(go/adbc): refactor logging instrumentation into OTel tracing - part 2/3 - #4659

Open
birschick-bq wants to merge 22 commits into
apache:mainfrom
birschick-bq:dev/birschick-bq/flight-sql-log-to-trace-4-connection
Open

feat(go/adbc): refactor logging instrumentation into OTel tracing - part 2/3#4659
birschick-bq wants to merge 22 commits into
apache:mainfrom
birschick-bq:dev/birschick-bq/flight-sql-log-to-trace-4-connection

Conversation

@birschick-bq

@birschick-bq birschick-bq commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

This pull request refactors the FlightSQL ADBC driver to improve error handling, tracing, and code clarity. The main focus is on replacing logger-based tracing with OpenTelemetry (OTel) tracing, enhancing context propagation, and ensuring errors are consistently handled and returned. Several methods now include OTel tracing spans, and error handling is standardized across the connection implementation.

Tracing and Observability Improvements

  • Replaced logger-based tracing (doGetWithLogger) with OTel-based tracing (doGetWithTracer) throughout the connection implementation, ensuring all relevant methods now emit OTel spans for better observability. ([[1]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL384-L472), [[2]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL855-R783), [[3]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1112-R1132))
  • Added OTel tracing spans to methods such as PrepareDriverInfo, GetObjectsCatalogs, GetObjectsDbSchemas, GetObjectsTables, and GetTableSchema, capturing start time, errors, and context for each operation. ([[1]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL821-R743), [[2]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL937-R873), [[3]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL971-R948), [[4]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1016-R984), [[5]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1096-R1081))

Error Handling and API Consistency

  • Standardized error handling by consistently capturing and returning errors, including context and metadata, across all public methods. This includes propagating errors from context checks and response metadata. ([[1]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL892-R832), [[2]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL955-R899), [[3]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL971-R948), [[4]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1016-R984), [[5]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1047-R1016), [[6]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1077-R1045), [[7]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1112-R1132), [[8]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1162-R1159))
  • Updated function signatures to use named return values and to properly set and return errors, improving clarity and reliability. ([[1]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL821-R743), [[2]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL937-R873), [[3]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL971-R948), [[4]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1016-R984), [[5]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1096-R1081), [[6]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1162-R1159))

Code Cleanup and Minor Fixes

  • Removed unused imports (such as log/slog) and the obsolete doGetWithLogger function. ([[1]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL26), [[2]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL384-L472))
  • Improved variable declarations and error assignments for better Go idiomatic style and code readability. ([[1]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL249-R250), [[2]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL842-R765), [[3]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL855-R783), [[4]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL955-R899), [[5]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1047-R1016), [[6]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1077-R1045), [[7]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1112-R1132), [[8]](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cL1162-R1159))

Test and Output Updates

  • Updated test assertions to match new trace output naming conventions, reflecting the refactored tracing approach. ([go/adbc/driver/flightsql/flightsql_adbc_test.goL371-R371](https://github.com/apache/arrow-adbc/pull/4659/files#diff-1147cdf8ab7a5dcb1d84bcb2b7a2185cf2fc3802b7eb3718bffadfe990ecdf56L371-R371))

Documentation and Comments

  • Added or updated comments to clarify the purpose of new and refactored functions, particularly around tracing and error handling. ([go/adbc/driver/flightsql/flightsql_connection.goR236-R237](https://github.com/apache/arrow-adbc/pull/4659/files#diff-5ea5525a7206089962734801d643f4aa6f553637d760a90255d54867270abc8cR236-R237))

These changes collectively improve the maintainability, observability, and robustness of the FlightSQL ADBC driver.Refactors logging instrumentation into OTel tracing


  • Integrate otelgrpc tracing
  • updates for flightsql connection
  • handle correct structured shutdown of database and tracing in correct order (tracingLifecycle)
  • ensures tracing provider not leaked on failed driver/database startup

Extends: #4655

Is part 2 of 3 changes.

@birschick-bq
birschick-bq requested a review from zeroshade as a code owner August 4, 2026 21:48
@birschick-bq birschick-bq changed the title Dev/birschick bq/flight sql log to trace 4 connection feat(go/adbc): refactor logging instrumentation into OTel tracing - part 2/3 Aug 4, 2026
@birschick-bq
birschick-bq marked this pull request as draft August 4, 2026 21:56
@birschick-bq
birschick-bq marked this pull request as ready for review August 19, 2026 01:01
Comment thread go/adbc/driver/flightsql/flightsql_adbc_test.go Outdated
Comment thread go/adbc/driver/flightsql/flightsql_connection.go
const spanName = "FlightSQL.Database.Close"
startTime := time.Now()
var span trace.Span
_, span = internal.StartSpan(context.Background(), spanName, d)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a refactor for later, but I wonder if StartSpan shouldn't also return a NewEndSpanHelper with the start time pre-configured

Comment thread go/adbc/driver/flightsql/flightsql_database.go Outdated
Unary: unaryTimeoutInterceptor,
Stream: streamTimeoutInterceptor,
},
{Stream: responseMetadataStreamInterceptor},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(We don't need to trace unary calls?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WIP

Comment thread go/adbc/driver/flightsql/flightsql_database.go Outdated
Comment on lines 1359 to 1365
if c.cl == nil {
return adbc.Error{
err = adbc.Error{
Msg: "[Flight SQL Connection] trying to close already closed connection",
Code: adbc.StatusInvalidState,
}
return err
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here...is the intent to make sure the deferred function sees the updated err? Because AFAIK, this isn't necessary.

https://go.dev/play/p/IuU7_RrPB_9

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's lots more instances of this in this PR that could be reverted.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this tries to refactor things from logging.go but duplicates it instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is duplicated only because the return types differ.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. It does bother me but it's unavoidable then.

@lidavidm lidavidm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though there's still many instances of unnecessary assignment before return that could be reverted

Comment on lines 1359 to 1365
if c.cl == nil {
return adbc.Error{
err = adbc.Error{
Msg: "[Flight SQL Connection] trying to close already closed connection",
Code: adbc.StatusInvalidState,
}
return err
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's lots more instances of this in this PR that could be reverted.

@birschick-bq

Copy link
Copy Markdown
Contributor Author

@lidavidm

LGTM, though there's still many instances of unnecessary assignment before return that could be reverted

I'll fix these and then give you a heads-up. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants